View port handler nonnil #2980
View port handler nonnil #2980
Conversation
Updating fork from main
Codecov Report
@@ Coverage Diff @@
## master #2980 +/- ##
==========================================
- Coverage 19.77% 19.64% -0.14%
==========================================
Files 113 113
Lines 13620 13508 -112
==========================================
- Hits 2694 2653 -41
+ Misses 10926 10855 -71
Continue to review full report at Codecov.
|
|
@jjatie is helping a lot. @danielgindi do you want to take these similar PRs about swift improvements? |
|
@liuxuan30 Now that you've looked at the other related PRs, is it okay for this to be merged? |
|
I‘m checking every day but can't guarantee I can review daily; I would try. Reviewing slowly right now, but I can get some merged for sure. |
|
I suggest in the future, similar improvements like remove optional can be squash into one PR, each commit could be one class or one topic to review, to reduce dependency and conflicts and makes it easier for you. |
|
Fixed |
| @@ -221,8 +221,7 @@ open class ChartViewBase: NSUIView, ChartDataProvider, AnimatorDelegate | |||
| _animator = Animator() | |||
| _animator.delegate = self | |||
|
|
|||
| _viewPortHandler = ViewPortHandler() | |||
| _viewPortHandler.setChartDimens(width: bounds.size.width, height: bounds.size.height) | |||
| _viewPortHandler = ViewPortHandler(width: bounds.size.width, height: bounds.size.height) | |||
There was a problem hiding this comment.
I think in some cases, setChartDimens() is useful rather than re-init again.
There was a problem hiding this comment.
In some cases it is, that is why the method still exists. In this case it is not useful, as the only thing that is happening is what already happens in the initializer I changed it to.
There was a problem hiding this comment.
This is the initialize method, so it does need to be inited here.
| @@ -12,21 +12,23 @@ | |||
| import Foundation | |||
| import CoreGraphics | |||
|
|
|||
| //@objc(ChartRenderer) | |||
There was a problem hiding this comment.
what is this, commented out code or documentation? If it's doc, maybe use doc style comment like /* or other
There was a problem hiding this comment.
That shouldn't be here at all. I was experimenting, but ultimately moved it all to another branch/PR. I missed this in the cleanup
| let viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
There was a problem hiding this comment.
empty line? Not sure if this is github display issue, but if old guard is deleted, no space line needed. (ignore if there is no empty line)
|
generally it's good, but I need to take a look at viewport handler myself if nil is fine. |
Removed extraneous comment Removed extra whitespace
|
Fixed based on comments |
| viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
| let viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
| let viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
| let viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
| { | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
This one should be here. The rest are now fixed
| viewPortHandler = self.viewPortHandler | ||
| else { return } | ||
|
|
||
|
|
|
Finished reviewing. |
No description provided.